-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
cc55e0d
to
94fda6e
Compare
671df6f
to
ddd0e59
Compare
448e950
to
c490c23
Compare
@mxnet-label-bot add [backend, C API, pr-work-in-progress] |
@leleamol Can you please review again to see if the implementation of the dlclose matches what you were thinking? @anirudh2290 Thanks for the help with the engine destructor info, can you take a look at the changes to the engines: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
LocalFree(err_msg); | ||
} | ||
#else | ||
*func = dlsym(handle, name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.. Sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This reverts commit 3112893.
elif (os.name=='nt'): | ||
lib = 'mylib.dll' | ||
|
||
fname = mx.test_utils.download('https://mxnet-demo-models.s3.amazonaws.com/lib_binary/'+lib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this test introduces a security vulnerability. Please replace it as soon as possible as this poses a security threat to our system as well as our users since this allows arbitrary code execution. I'd appreciate it if you would provide a replacement within the next 24h. #15755 is the corresponding revert PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested before to build this stub during the build process instead of loading from S3. See previous comment above: https://github.com/apache/incubator-mxnet/pull/15489/files#r308932209
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
Thanks @marcoabreu for the catch. As we discussed offline, the problem is not with the feature in this PR but with how test_library_loading is pulling from the S3 bucket in the CI. We'll work to build the library as part of the build so that it can be used internally in testing the CI without security risks. We'll move the test pulling from the S3 bucket to the examples directory so that it doesnt run in the CI. We may need some help @marcoabreu to get this working in the CI, since we'll need to build this library and then find the path where it lives during the test so we can pass the path to the In the meantime, we'll make these changes and then resubmit this PR. |
Could you elaborate? |
@marcoabreu we added an example directory at: example/lib_api. Currently the test.py includes a test that uses the locally built library (built using the included Makefile). But this only provides an example for how to build a library. We'll create another example python script that pulls from the S3 bucket and loads that one. But it will just be an example, not a unittest that will run in the CI. Do you have other ideas/suggestions on how we can improve testing or provide more examples? |
No pulling of any prebuilt binaries due to the same reasons, please. I understand you trying to make it more convenient, but I think that we're stepping over a line in that case. Give instructions how to compile the example and the user should be fine. Just to confirm: we will have a unit test running in ci at the end of the day, correct? |
@marcoabreu I think we're in agreement here. We will be distributing some pre-built libraries in an S3 bucket as examples. We will provide documented examples for how to pull and load these binaries. But these will not be tests that will be automatically run. They will be examples. Yes, we will have the unit test running in the CI that uses a library that is built in the CI. |
I have a suggestion. We can check the |
While that eliviates the problem from a ci perspective (if the bucket would be owned by the CI owners), it's still a red flag for our users. I'm afraid that companies prefer to stay away from the execution of an arbitrary native library that is downloaded from the internet - no matter whether it is backed by a hash or not. It's inevitable in some cases, but we want to reduce the possiblity of compliance scanners flagging us and thus making the usage of the MXNet more complicated than it has to be. I think the only acceptable way is if the library is getting produced as part of the build process that is entirely retrieved from source. These are also the tenants Apache employs - Apache project must not make any binary releases in any form. Thus let's best stick to that as well :) |
* Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu
* Dynamic Library Loading Support (#15489) * Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu * build library and stash it in CI * modifying unittest to use CI built binary * Retrigger CI * adding dlclose to initialize destructor * adding sample_lib target to all in Makefile
* Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu
* Revert "Dynamic Library Loading Support (apache#15489)" This reverts commit 3112893. * Retrigger CI * CI * retrigger CI * retrigger CI * retrigger CI
* Dynamic Library Loading Support (apache#15489) * Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu * build library and stash it in CI * modifying unittest to use CI built binary * Retrigger CI * adding dlclose to initialize destructor * adding sample_lib target to all in Makefile
* Dynamic Library Loading Support (apache#15489) * Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu * build library and stash it in CI * modifying unittest to use CI built binary * Retrigger CI * adding dlclose to initialize destructor * adding sample_lib target to all in Makefile
Description
This PR is part of a larger effort of adding new functionality to MXNet that was compiled separately from MXNet. See here and here for more info.
This PR is the first step that enables libraries to be dynamically loaded into MXNet at runtime .
Features:
This PR includes the following:
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments